Skip to content

Blogging Prompts: Add Core Data model and adjust service - #18607

Merged
dvdchr merged 15 commits into
trunkfrom
feature/18375-prompts-core-data
May 16, 2022
Merged

Blogging Prompts: Add Core Data model and adjust service#18607
dvdchr merged 15 commits into
trunkfrom
feature/18375-prompts-core-data

Conversation

@dvdchr

@dvdchr dvdchr commented May 13, 2022

Copy link
Copy Markdown
Contributor

Refs #18375

So... it's a pretty big chunk of changes. But most of it is Core Data, I promise! 😆 I originally intended to split the PR between adding just the Core Data model, and adding the changes to the BloggingPromptsService — but there's a naming conflict between the temporary model and the Core Data model so I decided to just fix it in one go.

A couple of notes about the changes:

  • Considering the size of changes and the fact that this touches Core Data, I'm thinking to target this for 20.0. I'm just hoping that this won't turn into a gigantic conflict after the release branch is cut 😬 .
  • Say goodbye to the temporary model. BloggingPrompt is now a Core Data model.
    • ⚠️ Note: I've added a siteID field to BloggingPrompt since the user can access multiple sites — but there shouldn't be any impact on the UI side since this has been handled internally in BloggingPromptsService. Just make sure that you're always getting the prompts from the service, or remember to add siteID when fetching the model directly.
    • Added a convenience method promptAttribution that returns BloggingPromptsAttribution.
  • To remove the references to examplePrompt, I've replaced the calls with the prompt instance variable in DashboardPromptCardCell, and CreateButtonCoordinator.
  • As for the changes on BloggingPromptsService, it now properly creates new objects and/or updates existing ones.
    • Additionally, I've added a convenience localTodaysPrompt method which synchronously fetches today's prompt from Core Data. I'm sure there's a use case for this somewhere.
  • Added some more unit tests.

To Test

Unit tests ⚙️

Make sure that the unit tests are passing.

Manual tests 👀

  • Enable the blogging prompts feature flag.
  • Verify that the prompt contents are displayed correctly in the dashboard card, FAB, and the post draft (after tapping the Answer Prompt).
Description Screenshot
Dashboard card dashboard-card
Floating action button create-post
Post draft post-draft

Regression Notes

  1. Potential unintended areas of impact
    N/A. Feature is not released yet.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A. Feature is not released yet.

  3. What automated tests I added (or what prevented me from doing so)
    Added unit tests for object mapping and date filtering.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

dvdchr added 11 commits May 13, 2022 00:06
@dvdchr dvdchr added this to the 20.0 milestone May 13, 2022
@dvdchr
dvdchr requested review from ScoutHarris and wargcm May 13, 2022 15:04
@dvdchr dvdchr self-assigned this May 13, 2022
@peril-wordpress-mobile

peril-wordpress-mobile Bot commented May 13, 2022

Copy link
Copy Markdown
Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@dvdchr dvdchr mentioned this pull request May 13, 2022
Closed
4 tasks
@dvdchr

dvdchr commented May 13, 2022

Copy link
Copy Markdown
Contributor Author

I'm pretty sure I've resolved the conflicts in d621615... 😅 . Giving it another go.

@wpmobilebot

wpmobilebot commented May 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor
You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18607-c3ca003 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot

wpmobilebot commented May 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor
You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr18607-c3ca003 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

ScoutHarris
ScoutHarris suggested changes May 13, 2022
View reviewed changes

@ScoutHarris ScoutHarris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how the tests are passing on the PR, but locally I'm getting failures on the dates like this:

Screen Shot 2022-05-13 at 12 48 51 PM

@dvdchr

dvdchr commented May 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ScoutHarris Ah, I see... I'm pretty sure this is a timezone issue. Since the dates under test are in UTC, calling dateAndTimeComponents() automatically converts the date to the machine's local timezone. The day component ended up moving 1 day back is most likely because your timezone is behind UTC. I should have used dateComponents(in timeZone: TimeZone, from date: Date) instead to extract the components so it ignores local timezone.

I'll post a fix soon! Thanks for noticing this. 🙇🏼

UPDATE: Should be addressed in c3ca003

@dvdchr dvdchr mentioned this pull request May 13, 2022
Merged
2 tasks
@dvdchr
dvdchr requested a review from ScoutHarris May 13, 2022 20:03
@ScoutHarris

ScoutHarris commented May 13, 2022

Copy link
Copy Markdown
Contributor

I saw one issue. I don't think it's related to this PR, but noting it here just in case.

The dashboard card doesn't update with the latest prompt. The compact card does, as does the prompts list, but the dashboard card still displays the previous prompts. I haven't looked, but I think it's related to Leandro's explanation of why pull-to-refresh doesn't update the card.

prompt_cards prompts_list

ScoutHarris
ScoutHarris approved these changes May 13, 2022
View reviewed changes

@ScoutHarris ScoutHarris left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the size of changes and the fact that this touches Core Data, I'm thinking to target this for 20.0.

I tend to agree. I don't see any PRs (at this time) that change the model, so should be ok until post-freeze. 🤞

:shipit: (eventually 😄 )

@dvdchr

dvdchr commented May 16, 2022

Copy link
Copy Markdown
Contributor Author

Thanks @ScoutHarris !

@dvdchr
dvdchr merged commit 0166ff8 into trunk May 16, 2022
@dvdchr
dvdchr deleted the feature/18375-prompts-core-data branch May 16, 2022 06:22
@ScoutHarris ScoutHarris mentioned this pull request May 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wargcm wargcm Awaiting requested review from wargcm
1 more reviewer
@ScoutHarris ScoutHarris ScoutHarris approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@dvdchr dvdchr

Labels

Blogging Prompts Core Data Issues related to Core Data

Projects

None yet

Milestone

20.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dvdchr @wpmobilebot @ScoutHarris